Fiet-440 - #20
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughAdded a new FetchAccountId action to obtain unique CEX account identifiers. Protobuf enum, server handler, constants, and a dev client test were updated; package version and a CCXT dependency were bumped. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant CEX_API as "CEX API (Bybit/MEXC/Binance)"
Client->>Server: ExecuteAction(FetchAccountId, cex, apiKey)
activate Server
alt Bybit path
Server->>CEX_API: privateGetV5UserQueryApi()
CEX_API-->>Server: { id, userID, ... }
Server->>Server: accountId = id, uid = userID
else MEXC path
Server->>CEX_API: spotPrivateGetUid()
CEX_API-->>Server: { uid, ... }
Server->>Server: accountId = uid, uid = uid
else Binance path
Server->>CEX_API: privateGetAccount()
CEX_API-->>Server: { uid, ... }
Server->>Server: accountId = uid, uid = uid
else Unsupported
Server-->>Client: INTERNAL error (unsupported broker)
end
Server->>Server: generate verityProof
Server-->>Client: { accountId, uid, verityProof }
deactivate Server
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/helpers/constants.ts (1)
3-4: Pre-existing issue: duplicate "fetchDepositAddress" entry.Lines 3-4 contain duplicate "fetchDepositAddress" entries. While this is a pre-existing issue (not introduced by this PR), consider removing the duplicate in a follow-up cleanup.
src/server.ts (3)
221-221: Address the type safety concern flagged in past reviews.Using
(broker as any)completely bypasses TypeScript's type checking. While necessary for dynamic method access, consider:
- Documenting why this is needed (these are exchange-specific private methods not in the CCXT type definitions)
- Creating a typed interface for the expected methods if this pattern grows
- Adding runtime validation (already suggested above)
Based on past review comments, this approach (checking method existence) was agreed upon, but the lack of response validation remains a concern.
257-265: Improve error logging to aid debugging.The error message doesn't indicate which method was attempted or what the actual error was. Include more context in the log and error response.
Apply this diff:
} catch (error) { - log.error(`Error fetching account ID ${cex}:`, error); + log.error(`Error fetching account ID from ${cex}:`, { error, cex, methods: { + bybit: typeof temp_broker?.privateGetV5UserQueryApi === "function", + mexc: typeof temp_broker?.spotPrivateGetUid === "function", + binance: typeof temp_broker?.privateGetAccount === "function" + }}); callback( { code: grpc.status.INTERNAL, - message: `Error fetching account ID from ${cex}`, + message: `Error fetching account ID from ${cex}: ${error instanceof Error ? error.message : 'Unknown error'}`, },
216-268: Consider rate limiting and caching for account ID fetches.Per the PR objectives, account ID proofs should be generated once (not cron-based) and stored. However, the current implementation doesn't include:
- Rate limiting to prevent abuse
- Caching to avoid redundant API calls for the same credentials
- Verification of required API permissions
While these may be handled at a higher level, consider adding protective measures to prevent hitting exchange rate limits.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
package.json(2 hunks)src/client.dev.ts(1 hunks)src/helpers/constants.ts(1 hunks)src/proto/node.proto(1 hunks)src/server.ts(3 hunks)
🔇 Additional comments (5)
package.json (1)
3-3: LGTM: Version bump is appropriate.The minor version increment correctly reflects the addition of a new feature (FetchAccountId action).
src/helpers/constants.ts (1)
12-12: LGTM: fetchAccountId correctly added to verity methods.The addition of "fetchAccountId" to CCXT_METHODS_WITH_VERITY is correct and aligns with the new FetchAccountId action that requires TLS proof verification.
src/client.dev.ts (1)
71-85: LGTM: Test correctly updated for FetchAccountId.The test appropriately:
- Removes
symbolandpayloadfields (not needed for account ID fetch)- Uses MEXC exchange for testing
- Applies optional chaining for safe property access
The changes correctly validate the new FetchAccountId functionality.
src/server.ts (2)
428-439: LGTM: Formatting adjustment without behavior change.The refactored ternary operation improves readability while maintaining the same logic: use
fetchDepositAddressif available, otherwise fall back tofetchDepositAddressesByNetwork.
224-228: The code is correct — no issue exists here.The Bybit API documentation confirms that the
/v5/user/query-apiendpoint returnsuserID(camelCase) as an integer field, exactly as the code uses it on line 227. This is not an inconsistency with MEXC or Binance—each exchange API uses its own field names (userIDfor Bybit,uidfor MEXC). The code correctly handles each API's distinct response structure.
| "@grpc/grpc-js": "^1.13.4", | ||
| "@grpc/proto-loader": "^0.7.15", | ||
| "@usherlabs/ccxt": "^0.0.12", | ||
| "@usherlabs/ccxt": "^0.0.13", |
There was a problem hiding this comment.
🧩 Analysis chain
Script executed:
npm view @usherlabs/ccxt@0.0.13 version 2>/dev/null || echo "Version 0.0.13 not found"Repository: usherlabs/cex-broker
<shell_result>
Repository: usherlabs/cex-broker
Exit code: 0
stdout:
Version 0.0.13 not found
</shell_result>
@usherlabs/ccxt@0.0.13 does not exist on npm registry.
The version specified in this PR is not available for download. As this change depends on the deployment of ccxt 0.0.13 (per the PR description), the package must be published to npm before this PR can be merged. Verify with the maintainers that the version has been released or coordinate the publication.
🤖 Prompt for AI Agents
In package.json around line 53, the dependency entry "@usherlabs/ccxt":
"^0.0.13" refers to a package version that does not exist on the npm registry;
update this to a valid published version or revert the version change. Ask the
maintainers to publish v0.0.13 before merging or change the dependency to an
existing released version (or a temporary fork/path) and update package.json
accordingly, then run npm install/npm ci and verify lockfile updates and CI
passes.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
3 participants
This PR is dependent on the approval and deployment of ccxt 0.0.13
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.